Skip to content

Gracefully handle file upload persistence errors by clearing state#275

Merged
alexluckett merged 29 commits intomainfrom
bugfix/handle-errors-in-file-upload
Jan 8, 2026
Merged

Gracefully handle file upload persistence errors by clearing state#275
alexluckett merged 29 commits intomainfrom
bugfix/handle-errors-in-file-upload

Conversation

@alexluckett
Copy link
Copy Markdown
Contributor

@alexluckett alexluckett commented Dec 2, 2025 •
edited
Loading

Proposed change

Files may disappear if they've expired (unexpected bugs with retrieveal keys, file expiry on S3, etc). Currently form submissions can go through with invalid file upload state as we don't re-check it before submission.

This PR:

  • clears state for problematic file upload questions (an exceptional scenario), rather than breaking the entire form submission.
  • moves file submission logic into a new onSubmit function within the component and removes special handling within SummaryPageController
  • removes the legacy definition.outputEmail feature, in favour of metadata.notificationEmail

Jira ticket: https://eaflood.atlassian.net/browse/DF-719

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Misc. (documentation, build updates, etc)

Checklist

  • You have executed this code locally and it performs as expected.
  • You have added tests to verify your code works.
  • You have added code comments and JSDoc, where appropriate.
  • There is no commented-out code.
  • You have added developer docs in README.md and docs/* (where appropriate, e.g. new features).
  • The tests are passing (npm run test).
  • The linting checks are passing (npm run lint).
  • The code has been formatted (npm run format).

@alexluckett alexluckett force-pushed the bugfix/handle-errors-in-file-upload branch from 91dd96c to ad767cd Compare December 2, 2025 18:08
@alexluckett alexluckett force-pushed the bugfix/handle-errors-in-file-upload branch from 1e59cc3 to 43b39ad Compare December 11, 2025 10:52
@alexluckett alexluckett force-pushed the bugfix/handle-errors-in-file-upload branch from a8f4c04 to ed79a7a Compare December 11, 2025 13:55
@alexluckett alexluckett force-pushed the bugfix/handle-errors-in-file-upload branch from ed79a7a to 17ac0b9 Compare December 11, 2025 13:58
@alexluckett alexluckett marked this pull request as ready for review December 15, 2025 09:22
@alexluckett alexluckett marked this pull request as draft December 15, 2025 09:27
@alexluckett alexluckett marked this pull request as ready for review December 16, 2025 09:28
alexluckett
alexluckett commented Dec 16, 2025
View reviewed changes
Comment thread src/server/plugins/nunjucks/context.js Outdated
alexluckett
alexluckett commented Dec 16, 2025
View reviewed changes
Comment thread src/server/plugins/engine/components/FileUploadField.ts Outdated
Comment on lines +301 to +302
const notificationEmail =
metadata.notificationEmail ?? 'defraforms@defra.gov.uk'
Copy link
Copy Markdown
Contributor Author

@alexluckett alexluckett Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the key changes

this used to be: definition.outputEmail and was swapped to metadata.notificationEmail.

Copy link
Copy Markdown
Contributor

@davidjamesstone davidjamesstone Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the fallback needed here?
We would'nt be inside this function if there was no notificationEmail given this line

@alexluckett alexluckett force-pushed the bugfix/handle-errors-in-file-upload branch from c390319 to 585a89f Compare December 17, 2025 10:54
@alexluckett alexluckett force-pushed the bugfix/handle-errors-in-file-upload branch from fc8ca08 to ee024f5 Compare December 17, 2025 12:47
@alexluckett alexluckett marked this pull request as draft December 17, 2025 12:48
@alexluckett alexluckett force-pushed the bugfix/handle-errors-in-file-upload branch from 02d3673 to 1db48cb Compare December 17, 2025 13:01
@alexluckett alexluckett marked this pull request as ready for review December 18, 2025 09:44
jbarnsley10
jbarnsley10 reviewed Dec 18, 2025
View reviewed changes
Comment thread src/server/plugins/engine/components/FileUploadField.ts
jbarnsley10
jbarnsley10 reviewed Dec 18, 2025
View reviewed changes
Comment thread src/server/plugins/engine/pageControllers/SummaryPageController.ts Outdated
jbarnsley10
jbarnsley10 reviewed Dec 18, 2025
View reviewed changes
Comment thread src/server/services/cacheService.ts Outdated
jbarnsley10
jbarnsley10 approved these changes Dec 18, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jbarnsley10 jbarnsley10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. My only question would be 'is it a sensible user experience where multiple files from say multiple upload pages have disappeared?' i.e. does it handle one error at a time, or all at once etc
Also there's a minor Sonar issue on a negated condition.
Approving

@alexluckett alexluckett force-pushed the bugfix/handle-errors-in-file-upload branch from a3d2fa4 to b23cc00 Compare December 18, 2025 10:33
davidjamesstone
davidjamesstone approved these changes Dec 18, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@davidjamesstone davidjamesstone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome - love it

Scullyon
Scullyon approved these changes Dec 18, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@Scullyon Scullyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good, just a minor comment in one of the files and also wondered the same thing as Jez.

Comment thread src/server/plugins/engine/pageControllers/errors.ts

getStateKeys() {
const extraStateKeys =
this.component instanceof FileUploadField ? ['upload'] : []
Copy link
Copy Markdown
Contributor

@Scullyon Scullyon Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in here or would the component itself be a better location for its own state keys? This method could then obtain the data from the component.

Copy link
Copy Markdown
Contributor Author

@alexluckett alexluckett Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good idea, that would be a better place to do it. I'll leave this as a follow-up though as this has been hanging around a bit over the holidays.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jan 8, 2026

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
96.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@alexluckett alexluckett merged commit 85fde67 into main Jan 8, 2026
23 checks passed
@alexluckett alexluckett deleted the bugfix/handle-errors-in-file-upload branch January 8, 2026 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidjamesstone davidjamesstone davidjamesstone approved these changes

@Scullyon Scullyon Scullyon approved these changes

@jbarnsley10 jbarnsley10 jbarnsley10 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alexluckett @davidjamesstone @Scullyon @jbarnsley10